Add SampleAspectRatioNum/SampleAspectRatioDen elements#1078
Add SampleAspectRatioNum/SampleAspectRatioDen elements#1078arch1t3cht wants to merge 1 commit intoietf-wg-cellar:masterfrom
SampleAspectRatioNum/SampleAspectRatioDen elements#1078Conversation
|
Hi, That mkvtoolnix issue link doesn't exist.
If players don't support the current specifications, I don't see why they would support new elements better. Maybe adding DisplayUnit = 5 for Sample Aspect Ratio (SAR) would be better. The specifications are probably not clear enough about what to do with cropping values when the DisplayUnit is 3 (Display Aspect Ratio). Some clarifications would also benefit a SAR value. |
|
The issue they refer to is problem this one (same issue number after the migration, topic fits). |
arch1t3cht
force-pushed
the
sample-aspect-ratio
branch
from
2347599 to
848877f
Compare
Currently, Matroska allows a video's sample aspect ratio to be set via
the display aspect ratio through the `DisplayWidth`/`DisplayHeight`
elements.
However, these elements are currently specified to encode the display
aspect ratio *after* applying cropping. This creates inconsistencies
with players that do not read the `PixelCrop` elements and, in theory,
makes it impossible for Matroska writers to create files that display
with the correct aspect ratio both in players that respect cropping and
in players that do not.
Moreover, many existing files do not actually follow this convention
that the display aspect ratio apply to the cropped video, and instead
store the display aspect ratio of the uncropped video. In particular,
MKVToolNix stores the `DisplayWidth`/`DisplayHeight` in this format.
In order to be able to resolve these discrepancies in the future
without changing existing behavior, this commit adds explicit
`SampleAspectRatio{Num,Den}` elements, which specify the sample aspect
ratio rather than the display aspect ratio. This has the advantage of
being independent of any cropping and is also consistent with the way
the sample/display aspect ratios are stored in video coding formats
like H.264 (see: ITU-T H.273, section 8.6 "Sample aspect ratio
indicator"). In cases where the `DisplayWidth`/`DisplayHeight` and the
`SampleAspectRatioNum`/`SampleAspectRatioDen` elements are both present
and might conflict, it is clearly specified in what way the sample
aspect ratio elements take precedence. Matroska writers can write both
kinds of elements without risking conflicts.
Refer to the following links for previous discussion about these issues.
See: https://codeberg.org/mbunkus/mkvtoolnix/issues/2389
See: mpv-player/mpv#13446
Apparently, mkvtoolnix moved to codeberg. It was this issue https://codeberg.org/mbunkus/mkvtoolnix/issues/2389 (snapshot of gitlab version, https://web.archive.org/web/20250327165455/https://gitlab.com/mbunkus/mkvtoolnix/-/issues/2389)
That's fair point, and that's why I initially suggested to retroactively updated spec to reflect reality of Matroska files that exist in the wild. In #947. Obviously, such amend to the existing elements, is controversial. In mpv media player, we have an The main issue is clearly outlined in the MKVToolNix issue. As currently defined in the Matroska specification, cropping is not backward compatible. The natural expectation would be that cropping is optional and applied only if supported. Otherwise, the uncropped image should be displayed. However, this is not the case. Adding cropping makes these files incompatible with unaware media players. This lack of backward compatibility is likely why adoption of the cropping has been slow and inconsistent, as users expect their files to work in Windows Media Player, MPC-HC, and VLC, regardless of whether cropping is applied or they see some black bars. |
Yes, indeed, thanks - the mkvtoolnix repository moved in the meantime. I updated the link.
Yes, a player that does not read the But the argument in favor of new elements would be that players like mpv that do read and apply the But, yes, if the goal is to fix the majority of existing files and players then #947 would accomplish that goal better, though it entails changing existing behavior.
I considered this; the main reason why I decided against it is that it results in somewhat confusing wording and inconsistent behavior, and possibly even break existing players. At the moment, But if everyone here prefers this over new elements I can of course change this. |
4 participants
Currently, Matroska allows a video's sample aspect ratio to be set via the display aspect ratio through the
DisplayWidth/DisplayHeightelements.However, these elements are currently specified to encode the display aspect ratio after applying cropping. This creates inconsistencies with players that do not read the
PixelCropelements and, in theory, makes it impossible for Matroska writers to create files that display with the correct aspect ratio both in players that respect cropping and in players that do not.Moreover, many existing files do not actually follow this convention that the display aspect ratio apply to the cropped video, and instead store the display aspect ratio of the uncropped video. In particular, MKVToolNix stores the
DisplayWidth/DisplayHeightin this format.In order to be able to resolve these discrepancies in the future without changing existing behavior, this commit adds explicit
SampleAspectRatio{Num,Den}elements, which specify the sample aspect ratio rather than the display aspect ratio. This has the advantage of being independent of any cropping and is also consistent with the way the sample/display aspect ratios are stored in video coding formats like H.264 (see: ITU-T H.273, section 8.6 "Sample aspect ratio indicator"). In cases where theDisplayWidth/DisplayHeightand theSampleAspectRatioNum/SampleAspectRatioDenelements are both present and might conflict, it is clearly specified in what way the sample aspect ratio elements take precedence. Matroska writers can write both kinds of elements without risking conflicts.Refer to the following links for previous discussion about these issues.
See: https://codeberg.org/mbunkus/mkvtoolnix/issues/2389
See: mpv-player/mpv#13446
This change is a second, different approach to solving the problem brought up in #947. I am, among other reasons, opening it to hopefully create more discussion on this issue: I'm not overly attached to this specific solution and would be equally happy if #947 was applied instead of this.
The advantages of this solution over #947 are:
DisplayWidth/DisplayHeightinclude cropped area #947 it's not clear how much backwards compatibility cropping: makeDisplayWidth/DisplayHeightinclude cropped area #947 would actually break since most files in the wild do not follow the current spec and follow what cropping: makeDisplayWidth/DisplayHeightinclude cropped area #947 proposes instead.The disadvantages of this solution over #947 are:
Making
SampleAspectRatio{Num,Den}completely "disable" (i.e. render informative) theDisplay{Width,Height}was chosen because even with addedSampleAspectRatio{Num,Den}elements there would still be no way to determine whether an existing file'sDisplay{Width,Height}are meant in the spec-compliant sense or in the spec-noncompliant-but-established sense.An alternative would be enforcing that
DisplayWidthbe automatically derived asDisplayHeight* (PixelWidth-PixelCropLeft-PixelCropRight) / (PixelHeight-PixelCropTop-PixelCropBottom) *SampleAspectRatioNum/SampleAspectRatioDen(and not be purely informative), overriding an existingDisplayWidthin the file if present. This would work for spec-compliant files, but would still display spec-noncompliant files with (the correct display aspect ratio but) incorrect display dimensions in spec-compliant players (Though players that currently ignore the spec and follow #947's proposal instead could opt to also adopt equivalent behavior here).Hence, completely disabling
Display{Width,Height}whenSampleAspectRatio{Num,Den}are present seems like the simpler choice, but I'm happy to change it if there are disagreements.